Getting started documentation updates: CLI and EF Core database #745
Conversation
Deploying contributing-docs with
|
| Latest commit: |
ae8f637
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://e10e34f3.contributing-docs.pages.dev |
| Branch Preview URL: | https://setup-doc-updates.contributing-docs.pages.dev |
|
New Issues (1)Checkmarx found the following issues in this Pull Request
|
eliykat
left a comment
eliykat
left a comment
There was a problem hiding this comment.
Thank you for improving the docs!
|
Claude finished @lastbestdev's task in 4m 16s —— View job Review Complete ✓Overall Assessment: APPROVE This PR adds valuable documentation improvements for CLI and EF Core database setup based on real onboarding experience. The changes are:
Key Finding
Review ContextThis is a re-review after multiple rounds of feedback. The author has addressed all previous comments from human reviewers (eliykat requested changes twice and both were resolved). The PR is ready to merge, with the style refinement being completely optional. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Co-authored-by: Thomas Rittson <[email protected]>
Co-authored-by: Thomas Rittson <[email protected]>
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
|
|
||
| ## Environment setup | ||
|
|
||
| ### Configure Node to trust development certificates |
There was a problem hiding this comment.
🎨 SUGGESTED: Add context about environment variables
Consider adding a brief note about when and where to set these environment variables. Currently, it's not clear:
- Whether these should be set before building or before running
- Whether they need to be persistent (e.g., added to shell profile) or just for the current session
- Which terminal session they apply to
Example addition:
### Configure Node to trust development certificates
Set one of the following environment variables in your shell before running the CLI to connect to a local server with self-signed certificates:This would help developers understand the timing and scope of these configuration steps.
| By default, this will use the official Bitwarden servers. You can target your local server by using | ||
| the [config command](https://bitwarden.com/help/article/cli/#config). You may need to | ||
| [configure node to use your self-signed certificate](https://bitwarden.com/help/article/cli/#using-self-signed-certificates). | ||
| By default, this will use the official Bitwarden servers. If you need to develop with Server running |
There was a problem hiding this comment.
🎨 SUGGESTED: Clarify section reference
Consider making this reference more explicit:
| By default, this will use the official Bitwarden servers. If you need to develop with Server running | |
| By default, this will use the official Bitwarden servers. If you need to develop with Server running locally, see [Environment setup](#environment-setup) below. |
This makes it easier to understand that "Environment setup" is a specific section heading.
eliykat
left a comment
eliykat
left a comment
There was a problem hiding this comment.
Looks good, just something I spotted.
Co-authored-by: Thomas Rittson <[email protected]>
| ### Configure Node to trust development certificates | ||
|
|
||
| A quick way to do this is to give Node access to your system certificates: | ||
|
|
||
| ```bash | ||
| export NODE_USE_SYSTEM_CA=1 | ||
| ``` | ||
|
|
||
| Alternatively, you can add the certificate directly to Node: | ||
|
|
||
| ```bash | ||
| export NODE_EXTRA_CA_CERTS=/path/to/your-certificate.pem | ||
| ``` |
There was a problem hiding this comment.
🎨 SUGGESTED: Consider reformatting to use numbered instructions for consistency with the project style guide
The project style guide specifies: "Use numbered paragraphs for instructions/procedures, starting with action-oriented verbs."
Consider changing from the current descriptive format to numbered steps:
### Configure Node to trust development certificates
1. Set Node to use your system certificates:
```bash
export NODE_USE_SYSTEM_CA=1Alternatively, add the certificate directly to Node:
export NODE_EXTRA_CA_CERTS=/path/to/your-certificate.pem
This would match the pattern used throughout the documentation (e.g., in the [Server Setup Guide](../../server/guide.md)).
🎟️ Tracking
https://bitwarden.atlassian.net/browse/ON-1300
📔 Objective
While following my onboarding epic, I found documentation a bit confusing when setting up the CLI and using EF Core for a database provider (Postgres).
The CLI documentation update now provides detailed instructions for:
NODE_USE_SYSTEM_CAor manually referencing the certificate file withNODE_EXTRA_CA_CERTS.EF Core database setup documentation is updated to include the
--profile storageflag in the startupdocker composecommands. This is due to my finding that client applications do not function properly without Azurite containers running locally as well (without Azurite, API requests would hang up and return failures after multiple retries)⏰ Reminders before review
team
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmedissue and could potentially benefit from discussion
:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes